-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Track more information about a note's status and consumability #355
Conversation
a025711
to
9a3dc69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a final review (still need to test locally), but left some comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
src/store/note_record/mod.rs
Outdated
/// Note is pending to be commited on chain | ||
Pending, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not for this PR, but I do think that we should eventually change Pending
to something like Expected
(i.e., the note is expected to appear on chain) to improve clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a PR to address this.
src/store/sqlite_store/store.sql
Outdated
status TEXT CHECK( status IN ( -- the status of the note - either pending, committed or consumed | ||
'Pending', 'Committed', 'Consumed' | ||
'Pending', 'Committed', 'Processing', 'Consumed' | ||
)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might ben an alternative way to do this: we keep the status
field as it was (with 3 variants), but instead change the consumer_transaction_id
into an array of transaction IDs (and probably rename it to consumer_transactions
).
The update logic would then be as follows:
- When a new transaction involving the note is submitted to the network, we append the corresponding transaction ID to the array. This way, we could track 2 or more in-flight transactions for the same note.
- The status remains as
Committed
.
- The status remains as
- Once a note is consumed, we remove all transactions IDs from the array except for the one for which the transaction was committed.
- We also change the status from
Committed
toConsumed
as we do now.
- We also change the status from
- If a transaction gets canceled (we can't detect this now, but in the future we may be able to determine this), we remove the transaction ID of the canceled transaction from the array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation would be something similar to the first iteration of this PR where the "Processing" status was implied and not stored in the database. I have a few questions:
- Do we want to let 2 accounts try to consume the same note with the same client? I could add a check that if a note has a
consumer_transaction_id
then the transaction is cancelled even before sending to the node. Clarifying just in case, theconsumer_transaction_id
only tracks transactions made by the same client. Transactions made by other clients are not tracked with that column. - If we receive the nullifier of a note with multiple
consumer_transactions
, how do we know which transaction ended up consuming the note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we receive the nullifier of a note with multiple
consumer_transactions
, how do we know which transaction ended up consuming the note?
This should be possible after #370. Basically, we'll get a list of committed transactions from the node as well. So, if we see a nullifier for a note, but the transactions we expected to consume the note is not in the list of committed transactions, the note must have been consumed by some other transaction.
Do we want to let 2 accounts try to consume the same note with the same client? I could add a check that if a note has a
consumer_transaction_id
then the transaction is cancelled even before sending to the node.
I'm wondering if I'm overthinking this a bit. With a centralized operator (something will have for the next 1 - 2 years), if a transaction is stale (i.e., has been sent to the node but is not getting committed) we can try to consume notes in a new transaction. If the node accepts the new transaction, we know that the stale transaction is canceled (i.e., dropped by the node for some reason), and so we can update its status and also replace consumer_transaction_id
with the ID of the new transaction.
This won't quite work in a decentralized setting (and that's when we might need to track more than one transaction ID per note) - but maybe we don't need to tackle this now.
To summarize: maybe it still makes sense to keep consumer_transaction_id
as a single value field. Note states can then be as follows:
expected
- we are expecting the note to appear on chain.
a. Here, it would also be useful to track the time at which the note was imported into the client (probably as a timestamp) so that we can tell how long have we been expecting this note to appear.committed
andconsumer_transaction_id
is NULL - the note is on chain, but we haven't executed a transaction to consume it yet.committed
andconsumer_transaction_id
is not NULL - we've executed a transaction to consume the note.consumer_transaction_id
tracks ID of the last transaction accepted by the node to consume this note.
a. Here, it would also be useful to track when the last transaction was submitted to the node so that we know how long we have been waiting for the transaction to be processed.consumed
- the note has been consumed. Ifconsumer_transaction_id
is set, it will be the ID of the transaction which consumed the note; if it is NULL, we don't know which transaction consumed the note.
a. Here, it might be useful to know when the note was consumed (i.e., in which block).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking on how can we represent all this additional information (timestamps, block heights, consumer ids, etc.). I started by just adding a new field in InputNoteRecord
for each one but that doesn't scale well.
One way would be to store specific information inside the NoteStatus
variants. Something like this:
pub enum NoteStatus {
/// Note is pending to be commited on chain.
Pending{tracking_start_timestamp: u64},
/// Note has been commited on chain
Committed{commit_height: u64},
/// Note has been consumed locally but not yet nullified on chain
Processing{consumer_account_id: AccountId, submition_timestamp: u64},
/// Note has been nullified on chain
Consumed{consumer_account_id: Option<AccountId>, nullification_height: u64},
}
The problem with this is that we wouldn't be able to access past information. Another way would be to add an optional struct with additional information but I feel that that would be too vague (at least with that name):
pub struct InputNoteRecord {
additional_information: AdditionalInfo,
}
pub struct AdditionalInfo{
tracking_start_timestamp: u64,
submition_timestamp: u64,
consumer_account_id: Option<AccountId>,
nullification_height: Option<u64>,
}
cc @igamigo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why we would need past data? In that case we can always factor out this status into its own table which maps Note IDs with NoteStatus
and a timestamp so we can know how it evolved.
In any case I think placing the data within the enum is a good approach. Although your suggestion does not reflect the transaction ID approach that Bobbin was mentioning. I think it could look something like this:
pub enum NoteStatus {
/// Note is pending to be commited on chain.
Pending{created_at: u64},
/// Note has been commited on chain
Committed{commit_height: u64},
/// Note has been consumed locally but not yet nullified on chain
Processing{transaction_id: TransactionId, submitted_at: u64},
/// Note has been nullified on chain
Consumed{consumer_transaction_id: Option<TransactionId>, block_height: u64},
}
or if we follow Bobbin's suggestion more directly (essentially removing processing
as a separate status):
pub enum NoteStatus {
/// Note is pending to be commited on chain.
Pending{created_at: u64},
/// Note has been commited on chain
Committed{commit_height: u64, consumer_transaction_id: Option<TransactionId>},
/// Note has been nullified on chain
Consumed{consumer_transaction_id: Option<TransactionId>, block_height: u64},
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, when Bobbin talks about removing processing
I thought he meant "removing it from the sql table" but to keep it as a variant. Both approaches would work.
Is there any reason why we would need past data?
Not that I can think of. Maybe it's a downside that wouldn't bother us. In any case I think the information would be stored in the SqliteStore
so we can always retreive it if we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why we would need past data?
This may be useful for audit/log purposes - but it is a "nice-to-have". So, if it simplifies things a lot, we can assume that we don't need this data.
Bobbin talks about removing
processing
I thought he meant "removing it from the sql table" but to keep it as a variant.
Originally, I thought about removing it from both - but given more recent discussion (about making the solution more tailored to the centralized operator for now), it may make sense to have the status on the Rust side, and maybe database side as well.
Another way would be to add an optional struct with additional information but I feel that that would be too vague (at least with that name)
On the database side, we could have a field called something like audit_log
which would keep track of some of this info (i.e., created_on
, submitted_on
etc.). We probably still want to keep status
as a separate field to simplify querying - but also not a strong preference from my end.
On the Rust side, another alternative is to turn the whole InputNoteRecord
into an enum based on the status. Something like this (though, I'm sure I missed some detail):
pub enum InputNote {
Expected(ExpectedInputNote),
Committed(CommittedInputNote),
ConsumedInputNote(ConsumedInputNote),
}
pub struct ExpectedInputNote {
note: NoteDetails,
created_on: Timestamp,
}
pub struct CommittedInputNote {
note: Note,
proof: NoteInclusionProof, // this also contains commit_height for the note
created_on: Timestamp,
transaction: Option<TransactionId>, // maybe submitted_on can come from the transactions table?
}
pub struct ConsumedInputNote {
note: Note,
proof: NoteInclusionProof,
created_on: Timestamp,
consumed_height: u32,
consumed_by: Option<TransactionId>,
}
Introducing Processing(ProcessingInputNote)
may make this cleaner still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up implementing it with the struct variants for NoteStatus
. I feel like if we add all the changes/features discussed here this PR will get really big. So I summerized the changes in the last comment, I can create an issue for each one and tackle them in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - this approach makes sense.
Just finished implementing the new additional information for the note status. I also modified the PR title and description to better reflect the new changes. I feel like all of the other changes discussed here should be implemented in separate PRs. These are the changes that are left unfinished:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I want to take a deeper look but it seems fine at a first glance.
consumer_transaction_id
from InputNoteRecord
if note gets consumed by another transaction
#379
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice Work! Left some comments.
Do you think we could do a similar thing for the note status as we did for, for example, NoteDetails? We could store the status as a not NULL json field. It'll probably clean up the serialization / deserialization code a bit as well.
created_at UNSIGNED BIG INT NOT NULL, -- timestamp of the note creation/import | ||
submitted_at UNSIGNED BIG INT NULL, -- timestamp of the note submission to node | ||
nullifier_height UNSIGNED BIG INT NULL, -- block height when the nullifier arrived |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could add constraints here, like
- if status is Committed or Processing, submitted_at should not be null
- if status is consumed, nullifier_height should not be null
I think we can do it in a follow up PR though, let's open an issue for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same thing for input notes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The submitted_at
may be null on a "Committed" status. Maybe the user just created the note and synced, in this case the note is Committed but it was never submitted to the node. What should be checked is that if a note is "Processing" then the submitted_at
can't be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Looks good! Tested locally and CLI output is neat! Worked with both public and private (with import/export) notes and worked without issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a small nit but we should be good to merge this after that.
src/store/note_record/mod.rs
Outdated
} | ||
/// Note is pending to be commited on chain. | ||
Pending { | ||
/// Timestamp (in seconds) when the note (either new or imported) started being tracked by the client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think it's worth mentioning here that this is a UNIX epoch-based timestamp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
…355) * Add Consuming note status * Infer Consuming status from store information * Add some useful comments * Add change to CHANGELOG * Add PR to CHANGELOG * Remove consuming notes from list consumable * Remove hardcoded note status names from sqlite store code * Remove `ConsumableNote` type * Add function to get the consumability of a note * Refactor `get_consumable_notes` * Rename `Consuming` to `Processing` * Fix clippy * Fix doc comments * Store `Processing` status in `SqliteStore` * Override `get_unspent_input_note_nullifiers` function from `Store` * Add note filter for `Processing` notes * Fix comment * Remove `Processing` status from sql table * Add additional info to `NoteStatus` enum * Refactor store/cli to use the new `NoteStatus` * Remove `consumer_account_id` from note record * Add additional information to string * Add new fields to database * Use local timezone for timestamp * Fix status display messages * Fix submitted typo * Fix tests * Improve doc comments for `NoteStatus` * Remove incorrect `unwrap_or` * Add `Pending` status to `SqliteStore` * Fix merge * Fix sqlite_store errors * Address suggestions
closes #328
This PR looks to add more information about a note status and consumability in the client. This is achieved by a few new features:
NoteStatus
variant calledProcessing
was created to represent the state betweenComitted
andConsumed
. It's a state where locally you executed a transaction to consume a note but the client doesn't know if the node confirmed it.NoteStatus
variants:Pending
: A timestamp of the creation of the note in the client.Commited
: The block height where the note was commited in the node.Processing
: The id of the consumer account of the note and the timestamp of the submition of transaction that is currently consuming the note.Consumed
: The id of the consumer account of the note (if known) and the block height where the note was consumed.get_note_consumability
) to allow the user to check a note's consumability (in addition to the previousget_consumable_notes
).Screenshot of the new status info: